-
-
Notifications
You must be signed in to change notification settings - Fork 307
Support isinstance
constraints in inference
#2846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2846 +/- ##
==========================================
+ Coverage 93.35% 93.39% +0.04%
==========================================
Files 92 92
Lines 11190 11208 +18
==========================================
+ Hits 10446 10468 +22
+ Misses 744 740 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
π New features to boost your workflow:
|
Seems like test coverage is lacking. Shouldn't be an issue to get it up to 100%, but I would like a review first to see if this implementation makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful!
As we do more of these, it would be nice to add a CI step that installs pylint and runs the primer. I had an experiment (work in progress) at jacobtylerwalls#157 and
pylint-dev/pylint@main...jacobtylerwalls:pylint:run-with-custom-astroid. Something for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobtylerwalls Should this be merged for 4.0.0 considering we already released a RC? Feels like a big change to include after a RC.
I wondered as well, happy to wait for 4.1. Let's release 4.0 final tomorrow in that case. |
Let's do so, especially as we haven't tested this with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat ! Glad to see that the constraint api is getting some love.
b3b9dd4
e5217a2
to
b3b9dd4
Compare
astroid/helpers.py
Outdated
except StopIteration as e: | ||
raise InferenceError(node=node, context=context) from e | ||
# arg2 MUST be a type or a TUPLE of types | ||
# for isinstance | ||
if isinstance(node_infer, nodes.Tuple): | ||
try: | ||
class_container = [ | ||
next(node.infer(context=context)) for node in node_infer.elts | ||
] | ||
except StopIteration as e: | ||
raise InferenceError(node=node, context=context) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StopIteration
handlers are not covered by tests, wondering if it is reasonable to exclude them. From what I understand .infer()
already throws InferenceError
and we won't get to these blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible for node.infer(context=context)
to be inferable but an empty iterable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was what I thought as well, but when we get an empty iterable, this decorator catches StopIteration
and raises InferenceError
? Not sure if there are cases where this decorator is not used.
Lines 75 to 96 in ab119c2
def raise_if_nothing_inferred( | |
func: Callable[_P, Generator[InferenceResult]], | |
) -> Callable[_P, Generator[InferenceResult]]: | |
def inner(*args: _P.args, **kwargs: _P.kwargs) -> Generator[InferenceResult]: | |
generator = func(*args, **kwargs) | |
try: | |
yield next(generator) | |
except StopIteration as error: | |
# generator is empty | |
if error.args: | |
raise InferenceError(**error.args[0]) from error | |
raise InferenceError( | |
"StopIteration raised without any error information." | |
) from error | |
except RecursionError as error: | |
raise InferenceError( | |
f"RecursionError raised with limit {sys.getrecursionlimit()}." | |
) from error | |
yield from generator | |
return inner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there are cases where this decorator is not used.
That worries me also (plugins?)
A pragma to exclude coverage seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I excluded coverage for those blocks, it is worth noting that they were previously uncovered as well. These changes only shift the method to a different file.
matches_checked_types = helpers.object_isinstance(inferred, types) | ||
|
||
if matches_checked_types is util.Uninferable: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Uninferable
check is not covered by tests, it should be unreachable in runtime. I am including it as a safe guard because helpers.object_isinstance()
returns bool | Uninferable
, but I am also fine with removing it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share more details about why this is unreachable? I see that we short-circuit if inferred is uninferable, but it wasn't apparent to me why inferring again wouldn't return uninferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked into object_isinstance()
, there are two main cases where it can return Uninferable
:
-
When the type of the object cannot be inferred, happens when:
- The object resolves to multiple types. This should not occur since
inferred
is an item within a list of inferred results. - Inference error is raised while doing
inferred.infer()
. This may occur wheninferred
itself isUninferable
, but as you mentioned there is already a short-circuit check above.
- The object resolves to multiple types. This should not occur since
-
When the type can be inferred but is not an instance of
nodes.ClassDef
. However, it seems that the inferred type always appears to be a ClassDef, or wrapped in a ClassDef proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not occur since inferred is an item within a list of inferred results.
That's what I'm unsure about, inferring an item may return something besides itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to think of a scenario where inferring a non-Uninferable inference result returns multiple results. Not sure if that's a common case but I'd be interested to know if you have any concrete examples in mind.
An alternative is to mock the method to return Uninferable
:
def test_isinstance_uninferable():
node = builder.extract_node(
"""
x = 3
if isinstance(x, str):
x #@
"""
)
with patch.object(helpers, "object_isinstance", return_value=Uninferable):
inferred = node.inferred()
assert len(inferred) == 1
assert isinstance(inferred[0], nodes.Const)
assert inferred[0].value == 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with that. Sorry I don't have the time to construct a concrete example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I will go with the mocking approach for now. If anyone has encountered this before, feel free to chime in!
b3b9dd4
to
010503c
Compare
010503c
to
16032d5
Compare
Type of Changes
Description
These changes implements the constraint interface to add a new constraint -
TypeConstraint
to improve the accuracy of inference for objects used in type-narrowing guards.Currently, the implementation only supports the
isinstance
pattern, but it should be relatively straightforward to extend it to support the negated case (not isinstance
) in the future.To determine whether an object is an instance of the given types, the implementation reuses existing inference mechanism for
isinstance
inbrains
.Refs pylint-dev/pylint#1162
Refs pylint-dev/pylint#4635
Refs pylint-dev/pylint#10469